Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Astria named forks #59

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

mycodecrafting
Copy link
Contributor

@mycodecrafting mycodecrafting commented Dec 4, 2024

astria "named forks" config changes which will allow Forma migration and allow dusk-11 to resume blocks.

@mycodecrafting mycodecrafting linked an issue Jan 14, 2025 that may be closed by this pull request
@mycodecrafting mycodecrafting marked this pull request as ready for review January 15, 2025 14:14
@mycodecrafting mycodecrafting changed the title WIP on "astria forks" config changes Astria named forks Jan 15, 2025
"feeCollector": "0xaC21B97d35Bf75A7dAb16f35b111a50e78A72F30",
"eip1559Params": { "minBaseFee": 0, "elasticityMultiplier": 2, "BaseFeeChangeDenominator": 8 },
"sequencer": {
"chainId": "sequencer-test-chain-0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just used for genesis validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a justfile in the dev directory too which is used in our tutorials on running a local rollup. These are the default values needed if you follow the tutorial.

RollupId: &rollupId,
SequencerChainId: fork.Sequencer.ChainID,
SequencerStartBlockHeight: fork.Sequencer.StartHeight,
SequencerStopBlockHeight: fork.Sequencer.StopHeight,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is stop height for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conductor uses stop height to determine when it needs to stop sending soft commits, wait for firm to reach the same level, and do an internal restart to re-fetch genesis info from geth which should have the next fork config

Comment on lines +222 to +225
// this fork halts the chain
if fork.Halt {
return nil, status.Error(codes.FailedPrecondition, "Block cannot be created at halted fork")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does the chain un-halt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manual intervention

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you link the doc for this? i'm unclear from the code as to how unhalting would work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes need to get some docs on this but basically the way it would work, such as the case of a sequencer network change, is you would pick some future height to halt at, update genesis to create a new named fork at that future height with halt set to true. Once the rollup reaches this height, it will stop. Then you do whatever manual steps you may need to do to prepare for the chain to migrate. e.g, updating conductor configs, taking any snapshots, etc. Then you update the geth genesis to set halt at this fork to false and set anything else you maybe did not know ahead of the halt, re-init and and start processing blocks.

)

type AstriaForks struct {
orderedForks []AstriaForkData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment saying these are in descending (?) order

return fmt.Errorf("fork %s: sequencer chain ID not set", fork.Name)
}

if fork.Sequencer.StartHeight == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can multiple forks on the same sequencer network have the same start height? i'm guessing not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes they can because we also track the RollupStartHeight. Forks inherit the config from the previous fork and only need to define values that are changing. Sequencer StartHeight only needs to be re-defined and be different in a fork if the network is changing or if the rollup wants to skip some sequencer blocks for some reason. Most typical forks will not change the sequencer config and it will carry forward from fork to fork.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay, i misunderstood the variable, this is the same sequencer start height as before, not where the fork starts

Comment on lines 264 to 266
idx := sort.Search(len(c.orderedForks), func(i int) bool {
return c.orderedForks[i].Height > height
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are multiple forks higher than height, this will return the overall highest, not the one directly higher than height (since orderedForks is descending by height)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. will fix

Comment on lines 252 to 271
func (c *AstriaForks) GetForkAtHeight(height uint64) AstriaForkData {
idx := sort.Search(len(c.orderedForks), func(i int) bool {
return c.orderedForks[i].Height <= height
})
// no named fork at this height
if idx == len(c.orderedForks) {
return GetDefaultAstriaForkData()
}
return c.orderedForks[idx]
}

func (c *AstriaForks) GetNextForkAtHeight(height uint64) *AstriaForkData {
idx := sort.Search(len(c.orderedForks), func(i int) bool {
return c.orderedForks[i].Height > height
})
if idx == len(c.orderedForks) {
return nil
}
return &c.orderedForks[idx]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add tests for these?

Comment on lines 337 to 339
if prefix != genesisPrefix {
return fmt.Errorf("bridge address must have prefix %s", genesisPrefix)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has to have the same prefix? what if the sequencer network was migrated and the prefixes are different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the address prefix is part of the AstriaSequencerConfig so can be changed in the genesis config if sequencer network changes. I think genesisPrefix is just leftover from the previous validation checks but the var name is confusing here. will update.

@mycodecrafting
Copy link
Contributor Author

@noot addressed all actionable items in 6bdcf95

@mycodecrafting mycodecrafting requested a review from noot January 16, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: named forks
2 participants